Skip to content

feat(codex): preserve opt-in provider-host circuit follow-up - #1039

Draft
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:agent/codex-host-circuit-preservation
Draft

feat(codex): preserve opt-in provider-host circuit follow-up#1039
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:agent/codex-host-circuit-preservation

Conversation

@luvs01

@luvs01 luvs01 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Status

Draft preservation PR — not merge-ready.

This preserves the optional provider-host circuit work that was separated from closed PR #922. PR #1023 merged as af100ec9 while this draft was being prepared, so the branch was rebased onto the post-#1023 dev line (4cfdc716 when opened) and now contains one follow-up commit only.

The planned continuation point has therefore arrived: completion will continue from the merged #1023 baseline. Before this is marked ready, it still needs maintainer direction on the public option/response contract, the full pre-push and cross-platform suite on the then-current dev, and resolution of any review findings.

What this preserves

  • Adds upstreamHostCircuitThreshold as an opt-in top-level setting. Default 0 leaves fix(codex): pre-connection transport failures no longer rotate pool accounts (#914) #1023's observational host ledger behavior unchanged; valid enabled values are 120.
  • Counts only terminal logical connect_neutral failures already proven by fix(codex): pre-connection transport failures no longer rotate pool accounts (#914) #1023's DNS/TCP reachability classifier. Timeouts, resets/TLS/unknown failures, and every HTTP response stay outside the circuit count.
  • Opens a 30-second provider-origin cooldown at the configured threshold, returns bounded 503 + Retry-After before pool account selection/upstream send, and admits exactly one half-open logical request after cooldown.
  • Uses generation-fenced leases so stale physical retries cannot mutate a newer circuit generation. A real HTTP response closes the admitted circuit; aborts and local failures release admission without adding host evidence.
  • Applies the opt-in gate to native OpenAI forward regular Responses and native compact sends, with unbound Pool admission before account selection and fixed/direct credential precedence preserved.
  • Documents the option in all five provider-reference locales and narrows fix(codex): pre-connection transport failures no longer rotate pool accounts (#914) #1023's host-ledger wording to the regular Responses/native compact paths it implements.

Deliberate limits

Validation on 19938291

The full repository suite and public CI are intentionally completion gates for moving this preservation draft to Ready, not claims made by this draft.

Related: #914, #922, #1023.

Summary by CodeRabbit

  • New Features

    • Added an optional provider-host circuit breaker for repeated pre-connection DNS/TCP failures.
    • Configurable thresholds support 0–20, with a 30-second cooldown and half-open recovery checks.
    • Open circuits return 503 with a Retry-After header and prevent unnecessary upstream retries.
    • Successful requests restore host health; timeouts and HTTP responses are excluded from circuit counting.
  • Documentation

    • Updated provider configuration guidance across supported languages.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds an opt-in provider-host circuit breaker for pre-connection DNS/TCP failures. Responses and compact requests now use admission leases, 30-second cooldowns, half-open probes, 503 responses, and lease-aware health settlement.

Changes

Provider-host circuit breaking

Layer / File(s) Summary
Health ledger and lease state
src/codex/upstream-host-health.ts, src/types.ts, tests/upstream-host-circuit.test.ts
Adds upstreamHostCircuitThreshold, admission leases, cooldown tracking, half-open probing, generation fencing, settlement, pruning, and comprehensive state tests.
Responses routing integration
src/server/responses/core.ts
Adds host admission for pooled routes, 503 handling, actual-host validation, lease-aware success and failure settlement, retry handling, and request cleanup.
Compact routing integration
src/server/responses/compact.ts
Adds equivalent admission, transport outcome handling, host verification, alternate-account handling, and final lease cleanup for native compact requests.
Configuration documentation and flow validation
docs-site/src/content/docs/*/reference/configuration/providers.md, tests/responses-compaction-routing.test.ts
Documents the setting and circuit behavior in five locales. Tests verify connection failure blocking, 503, Retry-After, no alternate account selection, and one upstream send.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant ResponsesCore
  participant CompactHandler
  participant UpstreamHostHealth
  participant Provider
  Client->>ResponsesCore: Send Responses request
  Client->>CompactHandler: Send native compact request
  ResponsesCore->>UpstreamHostHealth: Acquire host admission
  CompactHandler->>UpstreamHostHealth: Acquire host admission
  UpstreamHostHealth-->>ResponsesCore: Lease or 503 retry timing
  UpstreamHostHealth-->>CompactHandler: Lease or 503 retry timing
  ResponsesCore->>Provider: Forward admitted request
  CompactHandler->>Provider: Forward admitted request
  Provider-->>ResponsesCore: Response or connection failure
  Provider-->>CompactHandler: Response or connection failure
  ResponsesCore->>UpstreamHostHealth: Reset, record, or release lease
  CompactHandler->>UpstreamHostHealth: Reset, record, or release lease
Loading

Possibly related PRs

Suggested reviewers: lidge-jun, chrisae9

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 21.43% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately identifies the preservation of the opt-in provider-host circuit, which is the main change in the pull request.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the enhancement New feature or request label Aug 5, 2026
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs-site/src/content/docs/reference/configuration/providers.md`:
- Around line 25-26: Update the failover description beside
upstreamFailoverThreshold in all five listed provider configuration
files—docs-site/src/content/docs/reference/configuration/providers.md lines
25-26, docs-site/src/content/docs/ja/reference/configuration/providers.md lines
24-25, docs-site/src/content/docs/ko/reference/configuration/providers.md lines
24-25, docs-site/src/content/docs/ru/reference/configuration/providers.md lines
25-26, and docs-site/src/content/docs/zh-cn/reference/configuration/providers.md
lines 24-25—to replace “cooldowns” with the equivalent account-scoped wording
“account cooldowns,” while leaving the provider-host circuit description
unchanged.

In `@src/server/responses/compact.ts`:
- Around line 316-343: Extract the shared upstream-host circuit eligibility
predicate and 503 response builder currently defined in core.ts, export them,
and replace the inline ternary and blocked-admission response in the compact
handler with those helpers. Apply the same response-builder substitution to the
later blocked-admission path around the existing compact handler logic, while
preserving the compact endpoint’s current eligibility behavior and using the
shared helpers from both handlers.

In `@tests/responses-compaction-routing.test.ts`:
- Around line 936-960: Add an observable assertion that the second blocked
request in both circuit tests skips Pool account selection, such as spying on
the account-selection method or comparing Pool affinity/quota state before and
after that request. Keep the existing no-send and health assertions, and ensure
the test specifically verifies host admission prevents
resolveCodexAuthContext/account selection before returning 503.

In `@tests/upstream-host-circuit.test.ts`:
- Around line 142-173: Add a focused regression test near the existing
stale-completion tests that admits a lease for one upstream-host health key,
then attempts both failure recording and reset against a different key using
that lease. Assert the mismatched-key operations do not mutate the target host’s
state, and preserve coverage for both lease validation paths used by
recordUpstreamHostFailure and resetUpstreamHostHealth.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: bf0019f5-7f39-46c7-b85b-3d267bc70df9

📥 Commits

Reviewing files that changed from the base of the PR and between 4cfdc71 and 1993829.

📒 Files selected for processing (11)
  • docs-site/src/content/docs/ja/reference/configuration/providers.md
  • docs-site/src/content/docs/ko/reference/configuration/providers.md
  • docs-site/src/content/docs/reference/configuration/providers.md
  • docs-site/src/content/docs/ru/reference/configuration/providers.md
  • docs-site/src/content/docs/zh-cn/reference/configuration/providers.md
  • src/codex/upstream-host-health.ts
  • src/server/responses/compact.ts
  • src/server/responses/core.ts
  • src/types.ts
  • tests/responses-compaction-routing.test.ts
  • tests/upstream-host-circuit.test.ts

Comment on lines +25 to +26
| `upstreamFailoverThreshold?` | `number` | `3` | Consecutive transient failures before future new sessions fail over. Set `0` to disable. For regular Responses and native compact sends, proven pre-connection DNS/TCP reachability failures are tracked at the provider-host level: they never affect account health, cooldowns, thread/session affinity, active-account selection, or Pool routing, and never count toward this threshold. |
| `upstreamHostCircuitThreshold?` | `number` | `0` | Opt-in circuit threshold for proven pre-connection DNS/TCP failures on native OpenAI forward Responses and compact sends. `0` disables it; `1`–`20` opens a 30-second provider-origin cooldown after that many terminal logical requests. While open, requests receive `503` with `Retry-After` before account selection or upstream send; after cooldown, one half-open request is admitted. Timeouts and HTTP responses never count, and any HTTP response closes the circuit. |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Scope the failover statement to account cooldowns in every provider configuration page.

Each failover row says that the failures do not affect “cooldowns.” The following circuit row says that the same failures open a 30-second provider-origin cooldown. Replace the unqualified term with account-scoped wording.

  • docs-site/src/content/docs/reference/configuration/providers.md#L25-L26: Change “cooldowns” to “account cooldowns.”
  • docs-site/src/content/docs/ja/reference/configuration/providers.md#L24-L25: Use the equivalent account-scoped term in Line 24.
  • docs-site/src/content/docs/ko/reference/configuration/providers.md#L24-L25: Use the equivalent account-scoped term in Line 24.
  • docs-site/src/content/docs/ru/reference/configuration/providers.md#L25-L26: Use the equivalent account-scoped term in Line 25.
  • docs-site/src/content/docs/zh-cn/reference/configuration/providers.md#L24-L25: Use the equivalent account-scoped term in Line 24.

As per path instructions and the PR objective, the documentation must distinguish account failover state from the provider-host circuit state.

📍 Affects 5 files
  • docs-site/src/content/docs/reference/configuration/providers.md#L25-L26 (this comment)
  • docs-site/src/content/docs/ja/reference/configuration/providers.md#L24-L25
  • docs-site/src/content/docs/ko/reference/configuration/providers.md#L24-L25
  • docs-site/src/content/docs/ru/reference/configuration/providers.md#L25-L26
  • docs-site/src/content/docs/zh-cn/reference/configuration/providers.md#L24-L25
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs-site/src/content/docs/reference/configuration/providers.md` around lines
25 - 26, Update the failover description beside upstreamFailoverThreshold in all
five listed provider configuration
files—docs-site/src/content/docs/reference/configuration/providers.md lines
25-26, docs-site/src/content/docs/ja/reference/configuration/providers.md lines
24-25, docs-site/src/content/docs/ko/reference/configuration/providers.md lines
24-25, docs-site/src/content/docs/ru/reference/configuration/providers.md lines
25-26, and docs-site/src/content/docs/zh-cn/reference/configuration/providers.md
lines 24-25—to replace “cooldowns” with the equivalent account-scoped wording
“account cooldowns,” while leaving the provider-host circuit description
unchanged.

Source: Path instructions

Comment on lines +316 to +343
const preAuthCompactHostKey = normalizeUpstreamHostCircuitThreshold(
config.upstreamHostCircuitThreshold,
) > 0
&& route.codexAccountMode === "pool"
&& route.codexAccountId === undefined
&& route.provider.authMode === "forward"
? upstreamHostHealthKey(
route.providerName,
safeOriginLabel(route.provider.baseUrl ?? ""),
)
: null;
let compactHostAdmissionLease: UpstreamHostAdmissionLease | null = null;
let authCtx: CodexAuthContext = { kind: "main", accountId: null };
if (preAuthCompactHostKey) {
const admission = acquireUpstreamHostAdmission(
preAuthCompactHostKey,
config.upstreamHostCircuitThreshold,
);
if (admission.kind === "blocked") {
return formatErrorResponse(
503,
"upstream_host_circuit_open",
"Provider host is temporarily unavailable",
{ retryAfter: String(admission.retryAfterSeconds) },
);
}
compactHostAdmissionLease = admission.lease;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Extract the shared circuit eligibility predicate and the 503 builder.

This block duplicates policy that already exists in src/server/responses/core.ts:

  • eligibility: lines 316-326 here restate preAuthUpstreamHostCircuitKey (src/server/responses/core.ts lines 252-261) as an inline ternary.
  • 503 response: lines 335-340 here restate upstreamHostCircuitOpenResponse (src/server/responses/core.ts lines 263-270), and lines 420-425 restate it a third time.

The two eligibility expressions already differ. preAuthUpstreamHostCircuitKey adds route.provider.adapter !== "openai-responses"; this one relies on the enclosing supportsNativeResponsesCompactEndpoint guard at line 312 instead. The current behavior is equivalent, but a future change to circuit eligibility must now be applied in two places written in two different shapes. A change applied to only one place would silently diverge the Responses and compact circuits for the same provider host.

Export both helpers from a shared module and call them from both handlers.

♻️ Proposed consolidation

Export the two helpers from src/server/responses/core.ts (or a shared host-circuit module):

-function preAuthUpstreamHostCircuitKey(route: RouteResult, config: OcxConfig): string | null {
+export function preAuthUpstreamHostCircuitKey(
+  route: Pick<RouteResult, "provider" | "providerName" | "codexAccountMode" | "codexAccountId">,
+  config: OcxConfig,
+  options: { requireResponsesAdapter?: boolean } = {},
+): string | null {
   if (
     normalizeUpstreamHostCircuitThreshold(config.upstreamHostCircuitThreshold) === 0
     || route.codexAccountMode !== "pool"
     || route.codexAccountId !== undefined
     || route.provider.authMode !== "forward"
-    || route.provider.adapter !== "openai-responses"
+    || (options.requireResponsesAdapter !== false && route.provider.adapter !== "openai-responses")
   ) return null;
   return upstreamHostHealthKey(route.providerName, safeOriginLabel(route.provider.baseUrl ?? ""));
 }

-function upstreamHostCircuitOpenResponse(retryAfterSeconds: number): Response {
+export function upstreamHostCircuitOpenResponse(retryAfterSeconds: number): Response {

Then in this file:

-    const preAuthCompactHostKey = normalizeUpstreamHostCircuitThreshold(
-      config.upstreamHostCircuitThreshold,
-    ) > 0
-      && route.codexAccountMode === "pool"
-      && route.codexAccountId === undefined
-      && route.provider.authMode === "forward"
-      ? upstreamHostHealthKey(
-        route.providerName,
-        safeOriginLabel(route.provider.baseUrl ?? ""),
-      )
-      : null;
+    // The enclosing supportsNativeResponsesCompactEndpoint() guard already
+    // restricts this path to the canonical backends, so the adapter check
+    // from the Responses path is redundant here.
+    const preAuthCompactHostKey = preAuthUpstreamHostCircuitKey(route, config, {
+      requireResponsesAdapter: false,
+    });
     let compactHostAdmissionLease: UpstreamHostAdmissionLease | null = null;
     let authCtx: CodexAuthContext = { kind: "main", accountId: null };
     if (preAuthCompactHostKey) {
       const admission = acquireUpstreamHostAdmission(
         preAuthCompactHostKey,
         config.upstreamHostCircuitThreshold,
       );
       if (admission.kind === "blocked") {
-        return formatErrorResponse(
-          503,
-          "upstream_host_circuit_open",
-          "Provider host is temporarily unavailable",
-          { retryAfter: String(admission.retryAfterSeconds) },
-        );
+        return upstreamHostCircuitOpenResponse(admission.retryAfterSeconds);
       }
       compactHostAdmissionLease = admission.lease;
     }

Apply the same substitution at lines 420-425.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/responses/compact.ts` around lines 316 - 343, Extract the shared
upstream-host circuit eligibility predicate and 503 response builder currently
defined in core.ts, export them, and replace the inline ternary and
blocked-admission response in the compact handler with those helpers. Apply the
same response-builder substitution to the later blocked-admission path around
the existing compact handler logic, while preserving the compact endpoint’s
current eligibility behavior and using the shared helpers from both handlers.

Comment on lines +936 to +960
test("an opt-in regular circuit blocks before selecting another pool account", async () => {
await withPoolEnv("ocx-regular-host-circuit-", async config => {
config.upstreamHostCircuitThreshold = 1;
let sends = 0;
globalThis.fetch = (async () => {
sends += 1;
throw Object.assign(new Error("connection refused"), { code: "ECONNREFUSED" });
}) as typeof fetch;

const request = () => new Request("http://localhost/v1/responses", {
method: "POST",
headers: { "content-type": "application/json" },
body: JSON.stringify({ model: "gpt-5.6-sol", input: "hello", stream: false }),
});
const first = await handleResponses(request(), config, { model: "", provider: "" });
const second = await handleResponses(request(), config, { model: "", provider: "" });

expect(first.status).toBe(502);
expect(second.status).toBe(503);
expect(second.headers.get("retry-after")).toBe("30");
expect(sends).toBe(1);
expect(getCodexUpstreamHealth("pool-a")).toBeNull();
expect(getCodexUpstreamHealth("pool-b")).toBeNull();
});
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Make the no-account-selection assertion observable.

These tests prove that the blocked request sends no upstream request and creates no account-health record. They do not prove that the blocked request skips Pool account selection.

A regression could select pool-b, then return 503 before fetch. The current assertions would still pass. Add an account-selection spy or a before-and-after assertion on Pool affinity or quota state for the second request in both tests.

The supplied routing context places host admission before resolveCodexAuthContext in src/server/responses/compact.ts (Lines 258-417) and defines the eligible pre-auth route in src/server/responses/core.ts (Lines 252-261). The tests should protect that contract.

As per path instructions and the routing objective, this regression test must verify the no-account-selection behavior, not only the no-send outcome.

Also applies to: 962-988

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/responses-compaction-routing.test.ts` around lines 936 - 960, Add an
observable assertion that the second blocked request in both circuit tests skips
Pool account selection, such as spying on the account-selection method or
comparing Pool affinity/quota state before and after that request. Keep the
existing no-send and health assertions, and ensure the test specifically
verifies host admission prevents resolveCodexAuthContext/account selection
before returning 503.

Source: Path instructions

Comment on lines +142 to +173
test("a stale completion cannot mutate the generation that opened the circuit", () => {
const key = upstreamHostHealthKey("openai", "https://chatgpt.com");
const stale = admit(key, 1, 8_000);
fail(key, 1, 8_001);
const before = getUpstreamHostHealth(key);

recordUpstreamHostFailure(key, {
code: "ECONNREFUSED",
now: 8_002,
threshold: 1,
lease: stale,
});
expect(getUpstreamHostHealth(key)).toEqual(before);
});

test("a later physical retry without its lease cannot close a newer circuit", () => {
const key = upstreamHostHealthKey("openai", "https://chatgpt.com");
fail(key, 1, 9_000);
const before = getUpstreamHostHealth(key);

expect(resetUpstreamHostHealth(key, null, 9_001)).toBe(false);
recordUpstreamHostFailure(key, {
code: "ECONNREFUSED",
now: 9_002,
threshold: 1,
lease: null,
});
// Unwired observational callers are also unable to mutate circuit-owned state.
recordUpstreamHostFailure(key, { code: "ECONNREFUSED", now: 9_003 });

expect(getUpstreamHostHealth(key)).toEqual(before);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a regression test for the lease key mismatch guard.

The suite covers generation fencing, explicit-null settlement, and unleased observational writes. It does not cover the key mismatch guard.

Two source paths depend on that guard:

  • src/codex/upstream-host-health.ts line 253: if (!entry || opts.lease.key !== key) return;
  • src/codex/upstream-host-health.ts line 307: if (!entry || lease.key !== key) return false;

Both callers rely on it. src/server/responses/core.ts line 1817 and src/server/responses/compact.ts line 405 detect a host change after admission and return 502. If the key equality check regressed, a lease acquired for host A could settle health for host B, and the circuit state for B would become wrong while A kept a leaked lease. No current test would fail.

Add a focused case near the stale-completion tests.

💚 Proposed regression test
+  test("a lease cannot settle a different host key", () => {
+    const keyA = upstreamHostHealthKey("openai", "https://chatgpt.com");
+    const keyB = upstreamHostHealthKey("openai", "https://api.openai.com");
+    const leaseA = admit(keyA, 1, 10_000);
+
+    expect(resetUpstreamHostHealth(keyB, leaseA, 10_001)).toBe(false);
+    recordUpstreamHostFailure(keyB, {
+      code: "ECONNREFUSED",
+      now: 10_002,
+      threshold: 1,
+      lease: leaseA,
+    });
+    expect(getUpstreamHostHealth(keyB)).toBeNull();
+
+    // Lease A is still unsettled and retains authority over its own key.
+    recordUpstreamHostFailure(keyA, {
+      code: "ECONNREFUSED",
+      now: 10_003,
+      threshold: 1,
+      lease: leaseA,
+    });
+    expect(getUpstreamHostHealth(keyA)).toMatchObject({
+      consecutiveFailures: 1,
+      cooldownUntil: 10_003 + UPSTREAM_HOST_CIRCUIT_COOLDOWN_MS,
+    });
+  });

As per path instructions, "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
test("a stale completion cannot mutate the generation that opened the circuit", () => {
const key = upstreamHostHealthKey("openai", "https://chatgpt.com");
const stale = admit(key, 1, 8_000);
fail(key, 1, 8_001);
const before = getUpstreamHostHealth(key);
recordUpstreamHostFailure(key, {
code: "ECONNREFUSED",
now: 8_002,
threshold: 1,
lease: stale,
});
expect(getUpstreamHostHealth(key)).toEqual(before);
});
test("a later physical retry without its lease cannot close a newer circuit", () => {
const key = upstreamHostHealthKey("openai", "https://chatgpt.com");
fail(key, 1, 9_000);
const before = getUpstreamHostHealth(key);
expect(resetUpstreamHostHealth(key, null, 9_001)).toBe(false);
recordUpstreamHostFailure(key, {
code: "ECONNREFUSED",
now: 9_002,
threshold: 1,
lease: null,
});
// Unwired observational callers are also unable to mutate circuit-owned state.
recordUpstreamHostFailure(key, { code: "ECONNREFUSED", now: 9_003 });
expect(getUpstreamHostHealth(key)).toEqual(before);
});
test("a stale completion cannot mutate the generation that opened the circuit", () => {
const key = upstreamHostHealthKey("openai", "https://chatgpt.com");
const stale = admit(key, 1, 8_000);
fail(key, 1, 8_001);
const before = getUpstreamHostHealth(key);
recordUpstreamHostFailure(key, {
code: "ECONNREFUSED",
now: 8_002,
threshold: 1,
lease: stale,
});
expect(getUpstreamHostHealth(key)).toEqual(before);
});
test("a lease cannot settle a different host key", () => {
const keyA = upstreamHostHealthKey("openai", "https://chatgpt.com");
const keyB = upstreamHostHealthKey("openai", "https://api.openai.com");
const leaseA = admit(keyA, 1, 10_000);
expect(resetUpstreamHostHealth(keyB, leaseA, 10_001)).toBe(false);
recordUpstreamHostFailure(keyB, {
code: "ECONNREFUSED",
now: 10_002,
threshold: 1,
lease: leaseA,
});
expect(getUpstreamHostHealth(keyB)).toBeNull();
// Lease A is still unsettled and retains authority over its own key.
recordUpstreamHostFailure(keyA, {
code: "ECONNREFUSED",
now: 10_003,
threshold: 1,
lease: leaseA,
});
expect(getUpstreamHostHealth(keyA)).toMatchObject({
consecutiveFailures: 1,
cooldownUntil: 10_003 + UPSTREAM_HOST_CIRCUIT_COOLDOWN_MS,
});
});
test("a later physical retry without its lease cannot close a newer circuit", () => {
const key = upstreamHostHealthKey("openai", "https://chatgpt.com");
fail(key, 1, 9_000);
const before = getUpstreamHostHealth(key);
expect(resetUpstreamHostHealth(key, null, 9_001)).toBe(false);
recordUpstreamHostFailure(key, {
code: "ECONNREFUSED",
now: 9_002,
threshold: 1,
lease: null,
});
// Unwired observational callers are also unable to mutate circuit-owned state.
recordUpstreamHostFailure(key, { code: "ECONNREFUSED", now: 9_003 });
expect(getUpstreamHostHealth(key)).toEqual(before);
});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/upstream-host-circuit.test.ts` around lines 142 - 173, Add a focused
regression test near the existing stale-completion tests that admits a lease for
one upstream-host health key, then attempts both failure recording and reset
against a different key using that lease. Assert the mismatched-key operations
do not mutate the target host’s state, and preserve coverage for both lease
validation paths used by recordUpstreamHostFailure and resetUpstreamHostHealth.

Source: Path instructions

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: 1993829129

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant